Wire global model fallback chain on rate/usage limits + reset-aware cooldown - #2271
Wire global model fallback chain on rate/usage limits + reset-aware cooldown#2271lsm wants to merge 5 commits into
Conversation
Sessions hitting rate/usage caps (e.g. third-party relay 429 with a reset timestamp) never recovered: the watchdog retried the same model at a fixed 10min x3 then gave up, and GlobalSettings.fallbackModels/modelFallbackMap had no runtime consumers. - New pure fallback-recovery module: chain resolution (map override vs global), next-entry selection (skip tried / same-model / unavailable), format-agnostic reset-timestamp extraction (ISO-8601, YYYY-MM-DD HH:mm:ss incl. the Chinese relay shape, epoch s/ms), and a backoff ladder (10m->4h, cap 8h) with jitter. - RateLimitWatchdog drives two-phase recovery: (A) immediate fallback-model switch via injected deps (free, tracked per-episode), then (B) a cooldown at the parsed reset time or on the backoff ladder. Reset-known waits don't count toward maxAutoRetries. - AgentSession wires the watchdog to settings (chain), the provider registry (availability), model-switch (switch+retry after the failed query's finally), and the internal event bus (pause/resume). - Add session.rate_limit_pause / session.rate_limit_resume events.
…-resume With the fallback chain wired (previous commit), a 429 no longer fails a Space worker task — the error broadcast is skipped so the session recovers or waits. This commit adds the visible status: when a worker session pauses on a cap with no fallback left, mark its task rate_limited / usage_limited with a resume-at restriction, and restore it to in_progress when the limit lifts. - Migration 163 widens space_tasks.status CHECK (rate_limited, usage_limited) and adds a nullable restrictions column; test-DB helper kept in parity. - SpaceTaskStatus + SpaceTask.restrictions (TaskRestriction) in shared types; repo reads/writes the JSON blob. - TaskAgentManager subscribes to session.rate_limit_pause/resume and maps the session to its parent task, setting/clearing the paused status. - Web status maps (labels, badges, transitions) cover the two new statuses.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES
Reviewed from scratch: full diff, all 17 changed files, the integration points (query-runner.ts, model-switch-handler.ts, processing-state-manager.ts), and the surrounding space-runtime tick/rehydrate paths. Ran the new/changed unit suites (53 + 70, all green) and bun run check (lint/typecheck/knip/parity/session-guards/test-quality — all clean). The coder's verification claims hold.
What's solid. The pure recovery module (fallback-recovery.ts) is well-factored and correctly handles the Chinese relay shape (2026-07-22 17:55:10 via LOCAL_DATETIME_RE, the [1308] code correctly ignored), the backoff ladder, and the freeWait semantics. The concurrency core is sound: switchAndRetryForFallback correctly await this.queryPromise so the failed query's finally (null queryObject, env restore, setIdle) completes before handleModelSwitch runs — I traced no double-query / dedup / late-finally race, and cross-provider switching works (config-only branch swaps provider + clears sdkSessionId). Migration 163 follows the established M98/M162 table-rebuild pattern (FK-off, column copy, idempotent guard) and is safe. The onMarkApiSuccess → reset() → notifyResume resume path is correctly wired.
The findings below are real and actionable but none are crash/incorrect-output bugs — they concern reliability of the feature's headline guarantee and test coverage of its central invariants.
P2-1 — Auto-resume does not survive a daemon restart (the motivating use case)
The cooldown is an in-memory unref'd setTimeout (rate-limit-watchdog.ts:295). The persisted restrictions.resetAt blob (migration 163 + repo round-trip) is written on pause but never read on startup. The space-runtime tick loop only drives open/in_progress/blocked tasks (space-runtime.ts:4066), so a usage_limited/rate_limited task is never re-driven, and rehydrating a session constructs a fresh watchdog with no timer/retryCount.
Consequence: for a 5-hour or weekly cap — the exact scenario this PR exists to fix — a daemon restart during the wait leaves the task paused indefinitely with no auto-resume and no error. This is the same "persisted data with no runtime consumer" anti-pattern this PR removes for fallbackModels; the new resetAt repeats it. The manual Resume button (TaskStatusActions.tsx: rate_limited->in_progress) is an escape hatch, so it is recoverable, not a hard-stuck — but the "auto-resumes after reset" criterion (Part C / VERIFICATION) is unmet across restarts, and a paused task with a future resetAt that nobody arms is misleading in the UI.
Fix options: on daemon/workflow start, scan paused tasks — those with resetAt in the past → restore in_progress + clear restrictions; those still future → re-arm a runtime-level scheduled restore (or re-arm the watchdog cooldown on rehydrate). At minimum, explicitly document that auto-resume requires daemon uptime. The persisted resetAt should either be consumed or not persisted as a resume promise.
P2-2 — fireImmediateFallback has no error boundary; a rejection sticks fallbackPending = true
rate-limit-watchdog.ts:321-339 is void-fired from scheduleRetry:239 with no try/catch. switchAndRetryForFallback has a catch-all, but its own catch calls this.stateManager.setIdle() which can throw (DB write); the recursive await this.scheduleRetry(...) at :337 can also reject. If anything escapes, the promise rejects unhandled AND this.fallbackPending is never reset (:326 is skipped), so getState() reports 'fallback-pending' forever and retryNow() (:365) hard-returns false — a stuck-visible state with no recovery short of reset(). Wrap the body in try/catch: on error, log, clear fallbackPending, mark the entry tried, and fall through to a cooldown.
P2-3 — Untested load-bearing invariants
Three behaviors central to this feature have no test, so they will silently regress:
- Parsed-reset waits bypass
maxAutoRetries. This is the design guarantee ("reset-known waits don't count toward the budget"). The only exhaustion test (rate-limit-watchdog.test.ts:159) uses'429'(no timestamp) and assertsfalse; nothing asserts that withretryCount === maxAutoRetries+ a parseable ISO reset,scheduleRetryreturnstrueandretryCountstays put. - Late resume does not resurrect a cancelled/done task. Explicitly in the review checklist. The source guard (
restoreTaskFromRateLimit:610) is correct, but the only resume tests coverusage_limited→in_progress(:99) and thein_progressno-op (:141); nothing publishessession.rate_limit_resumeagainst acancelled/done/archivedtask. The terminal-override test (:129) only coversdoneon the pause side. resetAtbuffer arithmetic on thenotifyPausepayload is asserted nowhere (watchdog emitsdecision.retryAtMs= reset + 30s,:285); a regression dropping the buffer passes every test.
P3 — optional polish (not blocking on their own)
rate-limit-watchdog.ts:283firesnotifyPausebeforesetRateLimitCooldown(:289); reorder so the processing-state flip precedes the bus event and avoids a briefidlewindow whereonIdleCallbackruns.rate-limit-watchdog.ts:237comment says scheduleRetry "must return true synchronously" — it isawaited atquery-runner.ts:1314; rephrase to "must resolve to true".fallback-recovery.ts:301-312classifyLimitKindkeywords are broad ('exceeded','limit reached') and any parsed timestamp →usage_limit, so a transient "rate limit exceeded, retry in 60s" is labelled a daily/weekly cap. Label-only impact (both resume identically), but worth narrowing.- A manual status transition out of
rate_limited/usage_limitedviasetTaskStatus(not the resume path) leaves a stalerestrictionsblob; clear it on exit. No frontend consumer yet, latent. VALID_SPACE_TASK_TRANSITIONSomitsrate_limited/usage_limited → archived(every other non-terminal status can archive). Likely intentional; confirm.
Happy to re-review once P2-1 through P2-3 are addressed.
…ry, invariant tests P2-1 auto-resume across daemon restart: the in-memory watchdog cooldown doesn't survive a restart, so add a SpaceRuntime tick sweep (recoverRateLimitedTasks) that restores rate/usage-limited tasks whose persisted restrictions.resetAt has passed to in_progress (clearing restrictions), driven off the persisted blob so the normal rehydration restarts the worker. Tasks with a future resetAt stay paused. New repo helper listRateLimitedBySpace. P2-2 error boundary: wrap fireImmediateFallback in try/catch so a rejecting switchAndRetry or recursive scheduleRetry can't leave fallbackPending stuck true (which froze getState/retryNow). On error: clear fallbackPending, mark the entry tried, fall through to a cooldown. P2-3 invariant tests: parsed-reset wait bypasses maxAutoRetries (true + retryCount unchanged at the budget); 30s buffer pinned on notifyPause resetAt; late resume does not resurrect cancelled/done/archived; pause-side terminal guard covers done/blocked/cancelled/archived; cross-restart sweep test. P3 polish: setRateLimitCooldown before notifyPause; fix 'return true synchronously' comment; narrow classifyLimitKind keywords (drop 'exceeded'/ 'limit reached' that mislabelled transient rate limits); auto-clear stale restrictions on manual transition out of the paused statuses; add rate_limited/usage_limited → archived transitions + labels.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (NeoKai)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE
Round 2 (fresh, commit 755f340). All three P2 findings and the P3 polish are correctly addressed; I re-verified each on its merits rather than trusting the summary.
P2-1 (cross-restart auto-resume) — fixed. SpaceRuntime.recoverRateLimitedTasks() runs in the tick loop off the persisted restrictions.resetAt: past-reset → restore in_progress + clear restrictions (worker re-driven by the normal in_progress rehydrate/recoverStalledRuns path, which has separate coverage); future-reset → left paused. listRateLimitedBySpace + the repo's auto-clear of stale restrictions on any exit transition back it up. The only residual — a backoff-path rate_limited task carries a synthetic resetAt ≈ now+1h rather than the exact ladder delay — is safe: usage caps (the long-window case) always carry an accurate parsed reset, and a transient rate limit retrying slightly early is harmless. Not worth blocking.
P2-2 (error boundary) — fixed. fireImmediateFallback now wraps switchAndRetry in try/catch with a finally that always clears fallbackPending, and guards the recursive scheduleRetry with a best-effort cooldown fallback. No path leaves fallbackPending stuck or getState()/retryNow() frozen.
P2-3 (invariant tests) — fixed, and the assertions are meaningful: parsed-reset wait bypasses maxAutoRetries (returns true with retryCount unchanged at the budget); the 30s RESET_BUFFER_MS is pinned on the notifyPause payload; late resume does not resurrect cancelled/done/archived and confirms the restrictions auto-clear; pause-side terminal guard broadened to done/blocked/cancelled/archived.
P3 — all applied: setRateLimitCooldown before notifyPause; "must resolve to true" comment; classifyLimitKind narrowed (dropped the over-broad exceeded/limit reached/达到; the Chinese relay still classifies correctly via 上限/小时 and the parsed-reset path); rate_limited/usage_limited → archived transitions + labels.
Verification: bun run check clean (lint/typecheck/knip/parity/session-guards/test-quality); changed suites green (watchdog+fallback 55, listener+tick-loop+repo 125); all 4 prior threads resolved; PR open and mergeable. No new issues or regressions introduced by the fix commit. Ship it.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc71f95761
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Auto-resume tasks paused on a rate/usage cap whose reset has passed. | ||
| // Driven off the persisted `restrictions.resetAt`, so it survives daemon | ||
| // restarts (the in-memory watchdog cooldown does not). | ||
| await this.recoverRateLimitedTasks(); |
There was a problem hiding this comment.
On a daemon restart while restrictions.resetAt is still in the future, TaskAgentManager.rehydrate() excludes the paused task, but this tick calls processCompletedTasks() before this recovery sweep. Consequently, processRunTick() sees the persisted in_progress node execution without a live session, classifies it as crashed, resets it to pending, and respawns it; this sweep then skips the task because its reset time has not passed. The worker therefore resumes immediately while the task remains visibly rate-limited, bypassing the intended cooldown. Gate liveness/spawning for these paused statuses or handle them before processing runs.
Useful? React with 👍 / 👎.
| const ok = await Promise.resolve(p.isAvailable()); | ||
| if (!ok) return false; | ||
| if (typeof p.getAuthStatus === 'function') { | ||
| const auth = await p.getAuthStatus(); | ||
| return auth.isAuthenticated; |
There was a problem hiding this comment.
When a fallback targets anthropic-copilot authenticated through an environment variable, gh CLI, or hosts.yml, isAvailable() correctly returns true, but getAuthStatus() intentionally checks only HyperNeo-managed auth.json credentials (anthropic-copilot/provider.ts). Requiring both results makes an otherwise usable configured fallback appear unavailable, so the watchdog skips it and enters cooldown. Treat the successful runtime availability check as authoritative rather than additionally applying the UI-only authentication status.
Useful? React with 👍 / 👎.
| restrictions: { | ||
| type: status === 'usage_limited' ? 'usage_limit' : 'rate_limit', | ||
| limit: reason, | ||
| resetAt: resetAt ?? Date.now() + 60 * 60 * 1000, | ||
| sessionRole: 'worker', |
There was a problem hiding this comment.
For backoff-ladder cooldowns, notifyPause supplies no resetAt, so this stores an arbitrary one-hour deadline even though the watchdog's first delays are 10 and 30 minutes. If the daemon restarts during either step, the in-memory timer is lost and recoverRateLimitedTasks() trusts this persisted value, delaying recovery until an hour after the error. Persist the cooldown decision's actual retryAtMs for backoff waits instead of substituting a fixed hour.
Useful? React with 👍 / 👎.
| cancel(): void { | ||
| this.cancelCooldownTimer(); | ||
| this.fallbackPending = false; | ||
| this.notifyResume(); |
There was a problem hiding this comment.
When the user sends a new message or resets the session while fireImmediateFallback() is awaiting query teardown/model switching, cancel() only clears the flag and timer; it does not stop or invalidate that asynchronous operation. The fallback can therefore finish afterward, switch the reset session's provider, and re-enqueue the previously failed message alongside the user's new work. Track an episode generation or cancellation token and check it before switching and before re-enqueueing.
Useful? React with 👍 / 👎.
| rate_limited: ['in_progress', 'open', 'cancelled', 'archived'], | ||
| usage_limited: ['in_progress', 'open', 'cancelled', 'archived'], |
There was a problem hiding this comment.
For a workflow-backed paused task, these advertised manual transitions go through the ordinary setTaskStatus() path because isWorkflowRecoveryTransition() does not recognize either new source status, and the RPC's workflow-stop condition only recognizes in_progress/blocked. As a result, Resume merely changes the task row while the session remains in cooldown, and Cancel leaves the workflow/session running so the watchdog can later retry work on a cancelled task. The paused-source transitions need to cancel/retry the owning session and recover or stop the linked workflow rather than only updating status.
Useful? React with 👍 / 👎.
| | 'rate_limited' | ||
| | 'usage_limited'; |
There was a problem hiding this comment.
After a task enters either new status, it disappears from every Tasks tab: SpaceTasks.TAB_PREDICATES and TAB_GROUPS_DEF only partition draft, open/in-progress/approved, review/blocked, and terminal statuses, while the shared sidebar predicates omit these statuses as well. The new labels in the detail panes only help if the user already has a direct route open; normal task navigation and badge counts no longer expose the paused task or its manual recovery actions. Add both statuses to an appropriate task grouping and its shared predicates.
Useful? React with 👍 / 👎.
| // 1. ISO-8601 with timezone. | ||
| const iso = errorMessage.match(ISO_WITH_TZ_RE); | ||
| if (iso) { | ||
| const ms = parseIsoWithTzGroups(iso); | ||
| if (isValidReset(ms, now)) { | ||
| return { resetAtMs: ms, strategy: 'iso8601' }; | ||
| } |
There was a problem hiding this comment.
When an error contains more than one timestamp in the same format—such as a past request timestamp followed by a future quota reset—match() returns only the first token. If that token fails isValidReset, the parser never examines later ISO candidates and falls through to backoff even though a valid reset is present; the same problem affects the epoch patterns below. Iterate all candidates for each strategy until a plausible future reset is found.
Useful? React with 👍 / 👎.
| // (3) Re-enqueue with the new model and start a fresh query. | ||
| await this.executeRateLimitAutoRetry(lastUserMessage); | ||
| return true; |
There was a problem hiding this comment.
If the model switch succeeds but startQueryAndEnqueue() fails—for example because the replacement query did not start—executeRateLimitAutoRetry() catches that exception internally and returns normally, so this method reports true. The watchdog then treats the fallback as successful and stops advancing the chain or scheduling a cooldown, leaving the failed message idle with no recovery pending. Return a success value from the retry helper or let the failure propagate before returning true.
Useful? React with 👍 / 👎.
| // Don't override a terminal status the runtime has already decided on. | ||
| if ( | ||
| task.status === 'done' || | ||
| task.status === 'cancelled' || | ||
| task.status === 'archived' || | ||
| task.status === 'blocked' | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
A rate-limited sub-session can belong to a task currently in review or approved—notably a post-approval executor—but this guard allows those states to be overwritten with a limited status, and restoreTaskFromRateLimit() later always restores in_progress. That loses the approval/review lifecycle state and can bypass or abandon the pending post-approval route when the cooldown ends. Either restrict task-level pausing to tasks already in progress or persist and restore the exact prior status.
Useful? React with 👍 / 👎.
| await this.updateTaskAndEmit(space.id, task.id, { | ||
| status: 'in_progress', | ||
| restrictions: null, | ||
| }); |
There was a problem hiding this comment.
Paused statuses are excluded from getRunningTaskCount(), so while a task waits on a cooldown the runtime can use its freed maxConcurrentTasks slot to start another task. This unconditional cross-restart resume then marks the paused task in progress and restarts its worker without checking capacity, allowing the space to exceed its configured concurrency limit; the in-memory resume listener has the same unconditional behavior. Queue the resume until a slot is available or continue counting paused tasks against the limit.
Useful? React with 👍 / 👎.
The migration-marker runner tests seed a minimal space_tasks sentinel (no
status CHECK) to verify migrations are idempotent on bare schemas. runMigration163
threw 'space_tasks status CHECK constraint not found' there, failing
4-space-migrations-b in CI.
Align with M103's guard: only run the CHECK-widening rebuild when the live DDL
actually contains 'status IN ('; otherwise skip the rebuild and just add the
nullable restrictions column via ALTER (idempotent). Real pipeline-created
space_tasks tables have the CHECK, so the rebuild still runs there; bare
sentinel schemas get the column without the throw.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e05f3c8620
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const taskId = this.findParentTaskIdForSubSession(event.sessionId); | ||
| if (!taskId) return; | ||
| void this.restoreTaskFromRateLimit(taskId).catch((err) => { |
There was a problem hiding this comment.
When two parallel node-agent sessions belonging to the same task enter cooldown, the first session.rate_limit_resume event unconditionally restores the parent task and clears the single restriction even if the other session is still limited. This hides the remaining cooldown and, if the daemon restarts before it expires, removes the persisted reset deadline so the still-limited worker can be restarted immediately. Track limited session IDs per task and restore only after the last one resumes.
Useful? React with 👍 / 👎.
| | 'rate_limited' | ||
| | 'usage_limited'; |
There was a problem hiding this comment.
When a goal-owned task enters either new status, goal-service.ts:isActiveTaskStatus() still recognizes only open, in_progress, review, and approved. Consequently, createImmediateTaskInternal() or the scheduled-task claim path can clear the paused task from goal.activeTaskId and create or claim another task; the original task later auto-resumes, leaving two concurrent tasks for a goal whose active-task pointer is meant to enforce a single run. Include both limited statuses in the active-task predicate.
Useful? React with 👍 / 👎.
| this.lastErrorMessage = ''; | ||
| this.triedKeys.clear(); | ||
| this.chain = null; |
There was a problem hiding this comment.
QueryRunner.handleSDKMessage() invokes onMarkApiSuccess() for every SDK frame, including the initialization frame emitted when a fallback query starts, and this newly clears the tried-entry set and resolved chain. If fallback A then returns another 429, its episode has already been forgotten; after switching to B and receiving another initialization frame, B can select A again, producing an unbounded A/B fallback loop instead of exhausting the chain and entering cooldown. Reset the episode only after a meaningful successful turn, not initialization or error-result frames.
Useful? React with 👍 / 👎.
| // Mark the CURRENT (failed) provider+model as tried so we never re-select it. | ||
| const { provider, model } = this.deps.getCurrentModel(); | ||
| this.triedKeys.add(entryKey({ provider, model })); |
There was a problem hiding this comment.
The failed model is marked tried using its raw configured ID, while fallback entries also use raw IDs. For a session configured with an alias such as sonnet and a fallback entry containing the canonical ID for that same model, the entry is considered new; ModelSwitchHandler resolves both IDs, reports an already-current model as a successful switch, and leaves the alias in session config. Every subsequent 429 therefore selects the same canonical entry again and retries immediately forever. Deduplicate the current model and candidates using provider-aware resolved IDs.
Useful? React with 👍 / 👎.
| getCurrentModel: () => ({ | ||
| provider: (this.session.config.provider as string | undefined) ?? 'anthropic', | ||
| model: this.session.config.model ?? 'sonnet', | ||
| }), |
There was a problem hiding this comment.
For persisted sessions created before explicit provider IDs were stored, QueryRunner deliberately treats a missing provider as Anthropic, and this recovery code likewise resolves the chain as Anthropic. However, the actual fallback calls ModelSwitchHandler, which rejects immediately when session.config.provider is absent, so every configured fallback is reported as a failed switch and the legacy session falls through to cooldown. Persist or pass the inferred Anthropic provider before attempting the switch.
Useful? React with 👍 / 👎.
| this.config.taskRepo.updateTask(taskId, { | ||
| status, | ||
| restrictions: { | ||
| type: status === 'usage_limited' ? 'usage_limit' : 'rate_limit', | ||
| limit: reason, | ||
| resetAt: resetAt ?? Date.now() + 60 * 60 * 1000, | ||
| sessionRole: 'worker', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
These pause/resume paths write directly through taskRepo.updateTask() without publishing space.task.updated. The web task collections in packages/web/src/lib/space-store.ts are loaded by RPC and synchronized through that event rather than a task-list LiveQuery, so an already-connected client keeps showing the prior status and never receives the new restriction or its later removal until another refresh. Publish the updated task through the same event path used by SpaceRuntime.updateTaskAndEmit() after both writes.
Useful? React with 👍 / 👎.
1 participant
Sessions hitting 5h/weekly usage caps (e.g. third-party relay
429with a reset timestamp) never recovered: the watchdog retried the same model at a fixed 10min ×3 then gave up, andGlobalSettings.fallbackModels/modelFallbackMaphad no runtime consumers.This makes the configured fallback chain real and switches resets to format-agnostic parsing:
modelFallbackMap[provider/model]?? globalfallbackModels, switch to the next untried available entry via the existing model-switch machinery, and retry immediately. Repeated 429s advance through the chain; switches are free (don't count towardmaxAutoRetries).YYYY-MM-DD HH:mm:ssincl. the Chinese relay message, epoch s/ms) and wait until reset + buffer; otherwise use a backoff ladder (10m→4h, cap 8h, jitter). Reset-known waits don't count toward the retry budget.rate_limited/usage_limitedwith arestrictions.resetAtblob (migration 163) and auto-resumes toin_progresswhen the limit lifts.Pure recovery logic lives in a new
fallback-recovery.tsmodule (fully unit-tested); the watchdog is refactored to take injected deps.query-runner.ts/model-switch-handler.tsare unchanged — the existing skip-error-broadcast gate already prevents task failure on recovery.